Skip to content

refactor(container): one scorecard source, and a thin session route - #71

Merged
mariuspruvot merged 1 commit into
mainfrom
refactor/container-service-layer
Aug 1, 2026
Merged

refactor(container): one scorecard source, and a thin session route#71
mariuspruvot merged 1 commit into
mainfrom
refactor/container-service-layer

Conversation

@mariuspruvot

Copy link
Copy Markdown
Owner

The two architecture items deferred from #68, plus the xp_earned column that was dead since it was created.

Two extractors for one concept

Both ran inside finalize_session on every completed session:

source validated feeds
scorecard.extract_scorecard helprs-scorecard JSON block yes cs.scorecard (dashboard)
pr_comment.extract_score_card ## Results markdown, by regex no the PR comment

Same session, two formats, two failure modes. A small wording drift in a skill's markdown produced no PR comment at all while the dashboard scored the session fine — silently, since a missing match is indistinguishable from "no scorecard". And pr_comment.py ran its own select(SessionEvent.data) with a JSONB predicate to get there, inside a module whose sibling repository.py opens with "every query against them lives here".

Skills emit both formats deliberatelyskills/challenge-me/CLAUDE.md says "You MUST emit BOTH", markdown for the human watching the stream and JSON for anything that reads the outcome. So the fix is not to make skills emit less; it is to stop parsing the human-facing half. Only the JSON is parsed now, and the comment is rendered from the same validated object the dashboard stores.

The scorecard is a model

REQUIRED_FIELDS.issubset(...), len(dims) != 3, 0 <= value <= 10 were hand-rolled validation. That is a Pydantic model:

class Scorecard(BaseModel):
    model_config = ConfigDict(extra="allow")
    skill: str
    version: int
    dimensions: dict[str, float]
    summary: str
    ...

extra="allow" because the declared fields are the contract, not the ceiling — a skill reporting more should not be rejected.

Behaviour change worth flagging

The comment's headline score is now the mean of the reported dimensions rather than the free-text ### Score: 8 / 10 the skill wrote next to them. With the fixture's dimensions of 8/7/6 the comment says 7.0, where it previously echoed the skill's "8".

That is the point: nothing stopped the prose score from disagreeing with the dimensions on the line below it, and one of the two had to win. The computed one is the one that is checked.

xp_earned

Declared on the model, created by migration a2b3c4d5e6f8, exposed in two response schemas, consumed by containerApi.ts — and never assigned anywhere in src/. The API returned null every time. It is now the mean of the dimensions on a 0-100 scale, written alongside the scorecard, with an end-to-end test asserting it.

The route is thin again

create_container_session was 45 lines that resolved an installation, ran two authorization checks, fetched a BYOK config, Fernet-decrypted a credential, minted a scoped GitHub token, created a row, committed, started a container and closed the client. Credential decryption is not transport work, and the webhook path could never reuse any of it.

It is service.open_session now; the handler is 27 lines around one call.

Verification

  • 422 tests pass (was 419), ruff + ruff format + mypy clean
  • test_pr_comment.py rewritten against the new shape, including the validation gate that decides whether a comment is posted at all
  • The router fixture now carries both blocks a real skill emits — it previously modelled only the markdown half, which is why it did not catch the coupling

Two scorecard extractors coexisted and both ran inside finalize_session: a
validated JSON parser feeding cs.scorecard, and an unvalidated regex over
the markdown feeding the PR comment. Same session, two formats, two failure
modes — a small wording drift in a skill silently produced no comment while
the dashboard scored it fine, and pr_comment.py ran its own SQL over the
events table to do it, in a module whose sibling repository claims to own
every query.

Skills deliberately emit both formats: markdown for the human watching the
stream, JSON for anything that reads the outcome. Only the JSON is parsed
now. The comment is rendered from the same validated object the dashboard
stores, so the two cannot disagree about a session.

The scorecard is a Pydantic model rather than a dict with hand-rolled
checks, which also replaces `REQUIRED_FIELDS`/`len(dims) != 3`/`0 <= v <= 10`
with declarations. `extra="allow"`: the declared fields are the contract,
not the ceiling.

Behaviour change worth knowing: the comment's headline score is now the mean
of the reported dimensions instead of the free-text "Score: 8" the skill
wrote next to them — a number nothing stopped from disagreeing with the
dimensions on the line below it.

xp_earned is finally written, from that same scorecard. It was declared on
the model, created by a migration, exposed in two response schemas and read
by the frontend, and never once assigned.

create_container_session goes from a 45-line handler to 27 lines around one
call. Authorization, Fernet decryption and token minting were transport-
layer work that the webhook path could never reuse; they are `open_session`
in the service now.
@helprs-prod

helprs-prod Bot commented Aug 1, 2026

Copy link
Copy Markdown

helPRs session created for this PR.

Skill: challenge-me | Open session

@mariuspruvot
mariuspruvot merged commit 13c5fc7 into main Aug 1, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant